-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: add onKeyDown prop to ListBoxItem for custom keyboard handling
#9181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
onKeyDown prop to ListBoxItem for custom keyboard handlingonKeyDown prop to ListBoxItem for custom keyboard handling
snowystinger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting this, would you mind removing the docs and storybook in favour of a unit test? it can be added just after this describe block
| describe('press events', () => { |
|
Thanks for the review! I've addressed your feedback. Please take another look. |
|
This should be using |
| let {getAllByRole} = renderListbox({}, {onKeyDown}); | ||
| let options = getAllByRole('option'); | ||
|
|
||
| fireEvent.keyDown(options[0], {key: 'Delete'}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| fireEvent.keyDown(options[0], {key: 'Delete'}); | |
| await user.tab(); | |
| await user.keyboard('ArrowDown'); |
e7356f7 to
81ccc92
Compare
|
Fixed! I've updated the test to use userEvent.keyboard() instead of fireEvent.keyDown() to prevent event leaking 🙏 |
# Conflicts: # packages/react-aria-components/src/ListBox.tsx
|
|
||
| describe('onKeyDown', () => { | ||
| it('should call key handler when key is pressed on item', async () => { | ||
| let onKeyDown = jest.fn((e) => e.continuePropagation()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question to team, do we want to use useKeyboard for this, it stops events by default so we MUST call continuePropagation here for the "Escape" key to clear the selection.
This is not ideal because it causes the event to continue by default instead through all other event handlers. See my thoughts on this in the description of #8775
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ideal because it causes the event to continue by default instead through all other event handlers.
Hm, maybe I’m remembering something wrong but isn't that only an issue because the handlers further up do not consistently useKeyboard themselves?
If that were the case propagation wouldnt flow upwards uncontrollably, since it would be stopped at every level if not explicitly continued.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, I might've been thinking about it wrong. Looking at https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/createEventHandler.ts which would be called each time useKeyboard is used it looks like the previous continue would be overwritten and stop would be called.
I still do not like that users would need to know all the keys they should continue for the ListBox to work though. So I'm still not sure about using useKeyboard here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why a user wouldn't just call continue for any key, besides the ones with explicit purpose in his custom event handler. The same should be done in our parent handler, aka continue on everything we don't explicitly want to stop for. It basically translates to the exact thing you proposed in #8775, where there is an explicit intent of I do not want collisions to this key attached to any stop and yet we do not continue uncontrollably because we still need to think about calling continue explicitly every time we add a new handler.
I guess you could still argue that the user would have to know there is some keyboard interactivity that he may break if he doesn't continue in his handler, but I figure that would be discovered rather quickly.
Closes #8732
Added
onKeyDownprop toListBoxItemto support custom keyboard event handling for individual items,enabling features like deleting items with Delete/Backspace keys.
✅ Pull Request Checklist:
Issue.
Practices
📝 Test Instructions:
yarn start🧢 Your Project:
Open source contribution